fix: dependency installation gaps in Plugin Store and first-time install#385
Merged
Merged
Conversation
…ugin Store install_plugin/update_plugin (store_manager.py) installed requirements.txt with a bare `pip3` off PATH, bypassing the root-visible installer added in #380 for the "Reinstall Plugin Deps" button. Two bugs stacked: (1) `pip3` can resolve to a different Python install than the one that actually runs ledmatrix.service, and (2) even when it resolves correctly, ledmatrix-web runs as a non-root user so the package lands in that user's local site-packages, invisible to root-run ledmatrix.service. Either way the install reports success and writes the .dependencies_installed hash marker, so plugin_loader's own (correct) install-on-load path skips reinstalling — leaving the dependency permanently missing until a user finds and clicks the separate "Reinstall Plugin Deps" tool. This is why users kept hitting "No module named 'astral'" for the weather plugin even after installing it from the Store. Extracts the sudo-wrapper-then-fallback install logic from api_v3.py's _pip_install_requirements into src/common/permission_utils.py as install_requirements_file, and routes store_manager.py's dependency installation through it so the automatic Store install/update path now matches the manual "Reinstall Plugin Deps" path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesRequirements Installation Consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StoreManager as PluginStoreManager
participant ApiV3 as api_v3._pip_install_requirements
participant Helper as install_requirements_file
participant Wrapper as safe_pip_install.sh (sudo)
participant Pip as pip (user fallback)
StoreManager->>Helper: install_requirements_file(requirements_file, timeout=300)
ApiV3->>Helper: install_requirements_file(req_file, timeout)
Helper->>Wrapper: sudo -n bash safe_pip_install.sh req_file
alt sudo denied or wrapper missing
Helper->>Pip: sys.executable -m pip install --break-system-packages -r req_file
Pip-->>Helper: CompletedProcess
else wrapper handles install
Wrapper-->>Helper: CompletedProcess
end
Helper-->>StoreManager: CompletedProcess (check returncode)
Helper-->>ApiV3: CompletedProcess
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 11 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…quirements_file Codacy's generic subprocess-security rule (Bandit B603 equivalent) flagged the pip/sudo subprocess.run calls in install_requirements_file for lacking a "static string argument" — the standard pattern-based flag for any subprocess.run() call with a variable in its argv list. Both calls use list-form argv (no shell=True, so no shell-injection surface), and the only dynamic value is req_file, a Path built internally by callers rather than raw external input; safe_pip_install.sh independently re-validates it before installing anything as root. Suppresses with inline `# nosec B603` comments matching this codebase's existing convention (see permission_utils.py's own PROTECTED_SYSTEM_DIRECTORIES, display_manager.py, sync_manager.py, etc.). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
web_interface/requirements.txt and requirements.txt both pin requests>=2.33.0,<3.0.0, but Raspberry Pi OS ships an apt-managed python3-requests with no pip RECORD file. Upgrading it via plain `pip install` aborts with "uninstall-no-record-file" because pip refuses to uninstall a package it has no record of, in place — which is exactly the "Some web interface dependencies failed to install" warning first-time install hits. scripts/install_dependencies_apt.py and scripts/fix_perms/safe_pip_install.sh already work around this with --ignore-installed (lets pip lay the new version down in /usr/local, shadowing the apt copy, instead of trying to remove it first). first_time_install.sh's own direct pip invocations — the per-package requirements.txt loop, the web_interface/requirements.txt install, and the requirements_web_v2.txt fallback — didn't have it. Adds --ignore-installed to all three so first-time install no longer fails on this well-known apt/pip conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
ChuckBuilds
changed the title
The prior # nosec B603 comments suppressed Bandit's check but Codacy's
semgrep-based rule ("subprocess function 'run' without a static string")
kept flagging the same two lines as a critical security issue even after
that fix landed. install_dependencies_apt.py's _run() already needed both
tags together (# nosec B603 B607 ... # nosemgrep) for the identical
subprocess.run pattern, so apply the same double suppression here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/common/permission_utils.py (1)
383-386: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFallback install should include
--ignore-installed. The directpip installpath can still hit the apt-managedno RECORD file was foundfailure; keep it aligned withsafe_pip_install.sh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/permission_utils.py` around lines 383 - 386, The direct pip fallback in permission_utils.py should be aligned with safe_pip_install.sh by including --ignore-installed in the subprocess.run invocation used for the pip install path. Update the argument list in the install helper that builds the sys.executable -m pip command so it passes --ignore-installed along with --break-system-packages and -r req_file, keeping the behavior consistent for apt-managed packages that trigger the no RECORD file was found failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/common/permission_utils.py`:
- Around line 383-386: The direct pip fallback in permission_utils.py should be
aligned with safe_pip_install.sh by including --ignore-installed in the
subprocess.run invocation used for the pip install path. Update the argument
list in the install helper that builds the sys.executable -m pip command so it
passes --ignore-installed along with --break-system-packages and -r req_file,
keeping the behavior consistent for apt-managed packages that trigger the no
RECORD file was found failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ad434b9-48c1-42a6-b183-abdc52ec475f
📒 Files selected for processing (4)
first_time_install.shsrc/common/permission_utils.pysrc/plugin_system/store_manager.pyweb_interface/blueprints/api_v3.py
CodeRabbit review caught this (confirming a gap already flagged in conversation): the non-sudo fallback pip install in install_requirements_file was missing --ignore-installed, unlike the sudo-wrapper branch and safe_pip_install.sh. Without it, the same apt/pip RECORD-file conflict this PR fixes elsewhere (first_time_install.sh, install_dependencies_apt.py) could still hit installs that fall back to this path (e.g. a plugin's requirements.txt on a host where safe_pip_install.sh isn't set up yet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
6 tasks
ChuckBuilds
pushed a commit
that referenced
this pull request
Jul 8, 2026
Same generic Bandit/semgrep pattern-match on non-literal subprocess.run argv flagged in #385's install_requirements_file, now on the new --ignore-installed retry call added here: list-form argv (no shell=True), sys.executable is this process's own interpreter, and requirements_file is built internally by find_plugin_directory, never raw external input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
ChuckBuilds
added a commit
that referenced
this pull request
Jul 8, 2026
…conflicts (#386) * fix: plugin_loader retries with --ignore-installed before assuming apt package satisfies pin install_dependencies treated any "uninstall-no-record-file" pip failure as "dependency satisfied" and wrote the success marker without ever attempting --ignore-installed, unlike install_dependencies_apt.py and safe_pip_install.sh (added in #385 for the Plugin Store/first-time-install paths). A plugin pinning a newer version of a system-managed package (e.g. requests) would silently keep running against whatever version apt shipped, while the marker file claimed the pinned requirement was met. Now retries the same install with --ignore-installed on that specific failure so pip actually lays the pinned version down (shadowing the system-managed copy) before falling back to the prior tolerant behavior if the retry itself fails too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * chore: suppress Codacy finding on new retry subprocess.run call Same generic Bandit/semgrep pattern-match on non-literal subprocess.run argv flagged in #385's install_requirements_file, now on the new --ignore-installed retry call added here: list-form argv (no shell=True), sys.executable is this process's own interpreter, and requirements_file is built internally by find_plugin_directory, never raw external input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: tolerate a timed-out --ignore-installed retry, dedupe marker-write logic CodeRabbit review caught a real inconsistency: if the --ignore-installed retry itself timed out, subprocess.TimeoutExpired propagated to the outer handler and returned False, failing plugin load — contradicting the intended "tolerate this specific apt/pip conflict" behavior, where a mere non-zero retry return code already returns True. Wraps the retry in its own try/except so a timeout is logged and tolerated the same way as any other retry failure. Also extracts the marker-writing logic (open/write/chmod, ignoring OSError) into _write_dependency_marker, since it was duplicated identically between the direct-success path and the apt-conflict-retry-fallback path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx * fix: revert marker-write dedup helper, resolves CodeQL path-injection alert CodeQL flagged _write_dependency_marker's open(marker_file, ...) as "uncontrolled data used in path expression" (high severity) once the marker-write logic was extracted into its own method. marker_file is actually safe — it's built from safe_plugin_dir, which install_dependencies sanitizes via os.path.basename() (CodeQL's own recognized py/path-injection sanitizer, per the existing comment a few lines above) — but CodeQL's interprocedural analysis doesn't carry that sanitized status across the new method boundary, since the sanitizer call and the open() sink were no longer in the same function. This exact code produced zero CodeQL findings before the extraction (in two duplicated inline blocks) and is unchanged in what data reaches it — only its location moved. Reverting the extraction (keeping CodeRabbit's other, independent timeout-handling fix) restores the previously-clean shape rather than trying to convince the analyzer's cross-function taint tracking that a refactor changed nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx --------- Co-authored-by: Claude <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related dependency-installation bugs, both root-caused from real user reports:
1. Plugin Store install/update path installed dependencies with a bare
pip3store_manager.py'sinstall_plugin/update_plugin(the actual Plugin Store install/update path) installedrequirements.txtwith a barepip3offPATH, completely bypassing the root-visible installer added in #380 for the separate "Reinstall Plugin Deps" tool button. Two bugs stacked: (1)pip3can resolve to a different Python installation than the one that actually runsledmatrix.service, and (2)ledmatrix-web.serviceruns as a non-root user, so even a "successful"pip3install lands in that user's local site-packages, invisible to root-runledmatrix.service. Either way, the install reports success and writes the.dependencies_installedhash marker, soplugin_loader's own (correct) install-on-load path skips reinstalling — leaving the dependency permanently missing until someone finds and clicks the unrelated "Reinstall Plugin Deps" tool. This is why users kept hittingNo module named 'astral'for the weather plugin even after installing it from the Store.Extracted the sudo-wrapper-then-fallback install logic from
api_v3.py's_pip_install_requirementsintosrc/common/permission_utils.pyasinstall_requirements_file, and routed bothapi_v3.pyandstore_manager.pythrough it, so the automatic Store install/update path now matches the manual "Reinstall Plugin Deps" path.2. First-time install script fails on apt-managed
requestsweb_interface/requirements.txtandrequirements.txtboth pinrequests>=2.33.0,<3.0.0, but Raspberry Pi OS ships an apt-managedpython3-requestswith no pip RECORD file. Upgrading it via a plainpip installaborts withuninstall-no-record-file, producing exactly the "Some web interface dependencies failed to install" warning reported during first-time install.scripts/install_dependencies_apt.pyandscripts/fix_perms/safe_pip_install.shalready work around this with--ignore-installed;first_time_install.sh's own direct pip invocations (the per-packagerequirements.txtloop, theweb_interface/requirements.txtinstall, and therequirements_web_v2.txtfallback) didn't. Added--ignore-installedto all three.Type of change
Related issues
Related to #380 (fixes the automatic Plugin Store install/update path that #380 didn't cover).
Test plan
pytest test/test_plugin_loader.py test/test_store_manager_caches.py— 49 passed)main)bash -n first_time_install.sh(syntax check)Documentation
Plugin compatibility
Checklist
Notes for reviewer
Existing installs whose
.dependencies_installedmarker was already written by the old (broken)pip3path won't be retroactively fixed by the Plugin Store change alone, since the marker's hash still matches an unchangedrequirements.txt. Users currently affected can use the existing "Reinstall Plugin Deps" tool (Tools page) once, or reinstall the plugin, to pick up the corrected path.Codacy flagged the new
subprocess.runcalls ininstall_requirements_fileas a "critical" security issue (generic Bandit B603 pattern match on non-literal argv). Both calls use list-form argv with noshell=True, and the only dynamic value is an internally-constructedPath, never raw external input — the same pattern already used unmodified elsewhere in this codebase. Suppressed with inline# nosec B603comments matching this repo's existing convention.Summary by CodeRabbit